fix(subagent): retain failed review status after stream stall - #4748

Independent maintainer review at exact head ad25c508 — approved.

the bug and the fix

finalizeSubprocessOutput unconditionally reset exitCode to 0 and cleared stderr on any valid structured yield. So a review that produced public output and then hit a terminal stream failure was promoted to success — the worst possible direction for a failure to be wrong in.

Head passes done.error || done.aborted as an explicit terminal fact and keeps the nonzero exit and error while still swapping private raw output for the policy-safe yield (task/executor.ts:620-689,2191-2197). That is the right shape: the yield governs what is shown, the terminal fact governs whether it succeeded, and they no longer overwrite each other.

I traced the retain-too-much direction, since that is the other way this goes wrong

A retained failure that outlives its cause would be just as bad. It does not:

• A valid yield on a healthy run still clears the provisional nonzero code and succeeds.
• A valid yield after done.error/done.aborted retains the terminal exit and error.
• Schema-invalid and placeholder yields stay failures under existing validation.
• Cancel after a settled failure is a no-op (only running/paused/queued are cancellable), so the status stands.
• Pause after failure returns not_running without mutating.
• Resume without a message deliberately leaves terminal status and returns guidance.
• Resume with a message registers a fresh job, moves the predecessor to historicalJobIds, clears terminalGeneration, and repoints currentJobId/status (async/job-manager.ts:1583-1744). A successful run then writes completed and resultText.

So stale failure does not survive a later success, and the failure propagates through the task receipt and async job mapping as failed/errorText. After job eviction the canonical failed status remains on the record, and real executor runs keep the public payload in the agent:// artifact even though inline errorText belonged to the evicted job.

nit — two of the three new tests are guards

test/task/executor-warnings.test.ts:139-156 is the genuine pin: without the source change the old code sets exit 0 and clears stderr, so its final assertions fail.

test/tools/subagent.test.ts:94-124 synthesizes { kind: "failed" } directly, so it passes on the parent — AsyncJobManager already retained an explicitly returned failed outcome. packages/ai/test/idle-iterator.test.ts:81-121 supplies 300,000 ms explicitly and also passes on the parent; the existing stream-timeout-defaults.test.ts is what actually pins the xAI/Grok 300-second default.

Worth adding the lifecycle case that is currently unpinned: drive a terminal failure through finalization, resume with a message, and assert the current snapshot becomes running/completed with result text rather than stale error text. That is the clearing behavior I checked by reading, and it deserves a test.

_{Reviewed by @probepark — method: detached worktree at ad25c508, read of the finalization change against the previous unconditional reset, enumeration of every lifecycle transition that could retain or clear the failure (cancel, pause, resume with and without message, eviction), per-test base-vs-head discrimination. Tests not executed.}

gajae.pr-review-verdict.v1 merge-approved sha256:faadc5d118f1fe3a4acbc8cf7485b35fe97ee323a2c201089f96d60af4e7ae4e reviewer:human reviewer-id:probepark evidence:exact-head-ad25c508-terminal-fact-no-longer-overwritten-by-yield-and-resume-clears-stale-failure

Re-approved at exact head fee06539. My earlier approval was bound to ad25c508; the contract correctly invalidated it when the head moved. The new commit is a strict improvement.

Failed awaits are now visible (subagent-render.ts): the title becomes Subagent failed and the summary reports N subagents failed rather than a neutral count. That matters for this PR specifically — retaining the failed status is only useful if the failure surfaces.

progress is removed from await receipts (subagent.ts):

// AgentProgress includes model-generated deltas, tool arguments, nested
// task details, and arbitrary tool output. None is an approved public
// subagent payload, so await receipts expose only liveness.
return { liveProgressAvailable: true };

That closes a leak I did not catch last round: AgentProgress carried model-generated deltas, tool arguments, nested task details and arbitrary tool output into a parent-visible receipt. Reducing it to a liveness bit is the right call, and terminal public output still flows through the bounded result/error receipt and agent://.

Verified locally at this head: bun test packages/coding-agent/test/tools/subagent.test.ts packages/coding-agent/test/tools/subagent-render.test.ts packages/coding-agent/test/task/executor-warnings.test.ts — 100 pass, 0 fail.

Everything from my prior review still holds: the terminal fact is no longer overwritten by a valid yield, and resume-with-message clears stale failure rather than retaining it.

_{Reviewed by @probepark — method: detached worktree at fee06539, diff of the delta against my previously approved ad25c508, read of the receipt reduction against the AgentProgress shape, focused test run in a worktree with natives built.}

gajae.pr-review-verdict.v1 merge-approved sha256:20f9f5e3470812c5e756ce614d0080add22d687cc004c065d468ebea005496d9 reviewer:human reviewer-id:probepark evidence:exact-head-fee06539-delta-surfaces-failed-awaits-and-removes-agentprogress-from-receipts-100-pass

Re-approved at exact head 35f8210b — rebase only.

I diffed the PR's own two commits at this head against the same two at my previously approved fee06539, ignoring index and hunk headers: byte-identical. The head moved only because the branch was rebased onto dev and picked up #4612 (d97b79ef), which I merged a few minutes ago.

Nothing to re-verify. The prior approval's reasoning carries: the terminal fact is no longer overwritten by a valid yield, resume-with-message clears stale failure rather than retaining it, failed awaits now surface in the renderer title and summary, and AgentProgress no longer rides into await receipts.

Focused suites were 100 pass / 0 fail at the pre-rebase content.

_{Reviewed by @probepark — method: content-equality check of the PR's commits at fee06539 and 35f8210b against their respective parents, confirming a pure rebase over the newly merged #4612.}

gajae.pr-review-verdict.v1 merge-approved sha256:20f9f5e3470812c5e756ce614d0080add22d687cc004c065d468ebea005496d9 reviewer:human reviewer-id:probepark evidence:exact-head-35f8210b-rebase-only-content-byte-identical-to-approved-fee06539